Automatically retry HTTP requests returning 5xx

This commit implements auto-retry for downloading crates from crates.io whenever
a 5xx response is returned. This should help assist with automatic retries
whenever Cargo attempts to download directly from S3 but S3 returns a 500 error,
which is defined as "please retry again".

This logic may be a little eager to retry *all* 500 errors, but there's a
maximum cap on all retries regardless, so hopefully it doesn't result in too
many problems.

Closes #3962

diff --git a/src/cargo/sources/registry/remote.rs b/src/cargo/sources/registry/remote.rs
index 2edbd9e2fa0d6f527ed7ddf02a692100376cd0c8..7408eb450606eca9f5b8d95e10e62d38a9a04d87 100644 (file)
--- a/src/cargo/sources/registry/remote.rs
+++ b/src/cargo/sources/registry/remote.rs
@@ -16,6 +16,7 @@ use util::network;
 use util::paths;
 use util::{FileLock, Filesystem};
 use util::{Config, CargoResult, ChainError, human, Sha256, ToUrl};
+use util::errors::HttpError;
 
 pub struct RemoteRegistry<'cfg> {
     index_path: Filesystem,
@@ -153,26 +154,32 @@ impl<'cfg> RegistryData for RemoteRegistry<'cfg> {
         // TODO: don't download into memory, but ensure that if we ctrl-c a
         //       download we should resume either from the start or the middle
         //       on the next time
+        let url = url.to_string();
         handle.get(true)?;
-        handle.url(&url.to_string())?;
+        handle.url(&url)?;
         handle.follow_location(true)?;
         let mut state = Sha256::new();
         let mut body = Vec::new();
         network::with_retry(self.config, || {
             state = Sha256::new();
             body = Vec::new();
-            let mut handle = handle.transfer();
-            handle.write_function(|buf| {
-                state.update(buf);
-                body.extend_from_slice(buf);
-                Ok(buf.len())
-            })?;
-            handle.perform()
+            {
+                let mut handle = handle.transfer();
+                handle.write_function(|buf| {
+                    state.update(buf);
+                    body.extend_from_slice(buf);
+                    Ok(buf.len())
+                })?;
+                handle.perform()?;
+            }
+            let code = handle.response_code()?;
+            if code != 200 && code != 0 {
+                let url = handle.effective_url()?.unwrap_or(&url);
+                Err(HttpError::Not200(code, url.to_string()))
+            } else {
+                Ok(())
+            }
         })?;
-        let code = handle.response_code()?;
-        if code != 200 && code != 0 {
-            bail!("failed to get 200 response from `{}`, got {}", url, code)
-        }
 
         // Verify what we just downloaded
         if state.finish().to_hex() != checksum {

diff --git a/src/cargo/util/errors.rs b/src/cargo/util/errors.rs
index 3b1d35b6eee170189531d817072e3b93ca4f5f2d..7a6d0af1d412b4418fe99c3f059b43098527faf0 100644 (file)
--- a/src/cargo/util/errors.rs
+++ b/src/cargo/util/errors.rs
@@ -334,6 +334,7 @@ impl NetworkError for git2::Error {
         }
     }
 }
+
 impl NetworkError for curl::Error {
     fn maybe_spurious(&self) -> bool {
         self.is_couldnt_connect() ||
@@ -344,6 +345,63 @@ impl NetworkError for curl::Error {
     }
 }
 
+#[derive(Debug)]
+pub enum HttpError {
+    Not200(u32, String),
+    Curl(curl::Error),
+}
+
+impl fmt::Display for HttpError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match *self {
+            HttpError::Not200(code, ref url) => {
+                write!(f, "failed to get 200 response from `{}`, got {}",
+                       url, code)
+            }
+            HttpError::Curl(ref e) => e.fmt(f),
+        }
+    }
+}
+
+impl Error for HttpError {
+    fn description(&self) -> &str {
+        match *self {
+            HttpError::Not200(..) => "failed to get a 200 response",
+            HttpError::Curl(ref e) => e.description(),
+        }
+    }
+
+    fn cause(&self) -> Option<&Error> {
+        match *self {
+            HttpError::Not200(..) => None,
+            HttpError::Curl(ref e) => e.cause(),
+        }
+    }
+}
+
+impl CargoError for HttpError {
+    fn is_human(&self) -> bool {
+        true
+    }
+}
+
+impl NetworkError for HttpError {
+    fn maybe_spurious(&self) -> bool {
+        match *self {
+            HttpError::Not200(code, ref _url) => {
+                500 <= code && code < 600
+            }
+            HttpError::Curl(ref e) => e.maybe_spurious(),
+        }
+    }
+}
+
+impl From<curl::Error> for HttpError {
+    fn from(err: curl::Error) -> HttpError {
+        HttpError::Curl(err)
+    }
+}
+
 // =============================================================================
 // various impls